feat(rules): emit RunfilesGroupInfo from py_binary, py_test, and py_runtime - #3962
feat(rules): emit RunfilesGroupInfo from py_binary, py_test, and py_runtime#3962FrankPortman wants to merge 5 commits into
RunfilesGroupInfo from py_binary, py_test, and py_runtime#3962Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
5757fd5 to
432966d
Compare
There was a problem hiding this comment.
In general I quite like the solution, thank you for submitting the PR. I think @rickeylev is doing some experiments with the venv building so I'll defer to him for final approval. The only thing is that I would love to keep the @rules_runfiles_group//runfiles_group:enabled an internal target and force the users to go through the main flag in rules_python. THis makes it easier to reason about the API surface.
For my understanding:
- If this is disabled, the overhead would be minimal right? It's basically just an extra
ifcheck. - This is disabled by default?
- Where does the
rules_runfiles_groupdependency comes from?
One last thing, if we could link to upstream docs on how one can create this layering rule for container image, that would be a big +1 from me.
| When enabled (via `--@rules_runfiles_group//runfiles_group:enabled` or | ||
| {obj}`--@rules_python//python/config_settings:runfiles_groups`), `py_binary` |
There was a problem hiding this comment.
I think we should advertise only one - the //python/config_settings one.
…untime Implements bazel-contrib#3757: experimental support for the RunfilesGroupInfo provider from rules_runfiles_group 0.1.0. Emission is gated by two flags: the ecosystem-wide --@rules_runfiles_group//runfiles_group:enabled switch, and --//python/config_settings:runfiles_groups, whose default (auto) follows the ecosystem switch and whose explicit values override it for rules_python only. Off by default, zero cost when off. When enabled, binaries split their runfiles into named groups so packaging rules can build layered container images (shared interpreter layer, one layer per package): * rules_python#runtime: interpreter + stdlib (rank FOUNDATION, do_not_merge, kind foundation). py_runtime also emits this for its own runfiles. * rules_python#pypi/<name>: one group per PyPI package, detected via the dist-info METADATA file so group names stay stable even though whl repo names aren't (rank SHARED_DEPS, kind third_party). * One Label-named group per other py_library. * rules_python#venv: the binary-specific venv symlinks and support files. * rules_python#app: the binary's own code, executable, bootstrap and build data; named as the provider's executable_group. * One Label-named group per rule target in data attributes. Only binaries (and py_runtime) emit the public provider, because only they can satisfy its contract that the union of all groups equals DefaultInfo.default_runfiles exactly; analysis tests verify that. A py_library's sources reach the binary through PyInfo rather than its own runfiles, so libraries propagate their entries bottom-up through the private PyRunfilesGroupsInfo provider instead: entries reference dependencies' depsets rather than copying them, so per-target cost is constant. A consequence worth maintainer review: a foreign ruleset's binary with py_library deps gets no groups from them. Binaries that override pyc_collection away from the configuration default, or use the deprecated implicit __init__.py creation, fall back to a coarse runtime/venv/app grouping so the invariant still holds. Fixes bazel-contrib#3757
The checked-in lockfile validated by bzlmod_lockfile_test needs the BCR registry entries for rules_runfiles_group@0.1.0. Regenerated with the command from the test's README under Bazel 9.1.0.
The test bzl loaded @rules_runfiles_group directly, but that repo is only defined where the feature is available (bzlmod, Bazel 9+). Route the loads through the runfiles_groups_shim like the rule code does. The flag label from the shim is canonicalized via Label() so rules_testing's transition can resolve it.
dab6888 to
fcba61a
Compare
The generated py_library targets gained the _runfiles_groups_flag implicit attribute, so add it to the expected dep set. The upstream flag attribute is filtered out like toolchain types are: the shim points it at @rules_runfiles_group only on Bazel 9+, and at //python:none (already in the set) elsewhere.
|
Thanks for the review! Re the flag, I deliberately built an interaction with the upstream flag but this definitely up for discussion. The upstream
My thought was that it would be nice to not have to opt individual rulesets in, if we're comfortable doing a top-down Your questions:
I'll add pointers to the |
|
I'd love @malt3 have a look as well, does this look idiomatic usage of the |
|
Hey there. I'd like to give this a proper review. This week I'm on the go without a laptop. I'll be able to review early next week. |
FrankPortman
left a comment
FrankPortman
left a comment
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
Adversarial review of the current head (c37ba6dd), findings inline. Summary and the parts that don't fit on a line:
Verdict. Not mergeable as-is: the public provider's core contract (union of groups == default_runfiles) is violated for any rule target in data whose default outputs aren't in its data_runfiles (verified empirically on Bazel 9.1.1; details on the collect_data_entries comment), and the test suite cannot detect it. The design choices themselves check out: the private PyRunfilesGroupsInfo seam, using data_runfiles for data, both coarse fallbacks (verified to hold via cquery), and the version-gated shim.
Structural feedback / simplifications.
- The groups are built by re-enumerating every runfiles contribution in parallel with the code that builds
DefaultInforunfiles, so the invariant holds by bookkeeping rather than by construction. The robust shape is to build the runtime/venv/app/dep pieces once in_get_base_runfiles_for_binary/_create_executableand derive bothDefaultInfoand the groups from the same objects. This is the one item worth a maintainer opinion before acting, since it touches core runfiles construction. - Fixing the blocker is a simplification:
datahandling collapses tofilesmerged withdata_runfilesfor every target, and the singleton heuristic disappears. - Deduplicate the pyc-selection rule with
PycCollectionAttrand extract one shared "PyInfo files a binary adds" helper used by both the binary runfiles loop and the groups code. - Drop the duplicated rank constants, inline the single-use provider wrapper, skip empty entries, and declare the flag attrs only when the feature is available.
Performance. Disabled mode: two extra implicit label attrs per py target plus a couple of branches, and one avoidable runfiles allocation per binary (venv_files_runfiles). Enabled mode: per-target cost is O(direct deps) with one entry and one depset node retained (linear, not quadratic, thanks to the 0.1.0 depset shape); no new actions. The place it can bite at scale is consumer-side: per-library granularity plus the empty entries mean a binary with N transitive libraries yields roughly N+5 groups, and a packager enforcing a layer budget pays upstream's pairwise merge, roughly O(G^2) in group count. Skipping empty entries helps now; a coarser (e.g. by-repo) granularity option is a reasonable follow-up.
Test gaps (listed inline on the test file): both coarse fallbacks, py_test, py_runtime, pyi_deps, precompile-enabled selection, py_binary-in-deps, foreign RunfilesGroupInfo deps, non-PyInfo deps, rule targets in srcs, --build_python_zip.
CI is currently red on the ruff format check (one file).
| if RunfilesGroupInfo in target: | ||
| transitive.append(target[RunfilesGroupInfo].entries) | ||
| continue | ||
| runfiles = target[DefaultInfo].data_runfiles |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
blocker / correctness. Union of groups != default_runfiles for the common data shape. With Bazel's default --incompatible_always_include_files_in_data, ctx.runfiles(collect_default = True) adds a data rule target's default outputs in addition to its data_runfiles; this only adds data_runfiles (plus the singleton special case below), and the docstring asserts default outputs "contribute nothing". Verified on Bazel 9.1.1 with a rule returning DefaultInfo(files = depset([a, b])) and a rule with files != runfiles in data: their files land in default_runfiles and in no group, via py_binary data, py_library data, and inherited through py_binary-in-deps. Upstream's runfiles_groups.data_entry() gets this right (files merged with runfiles).
Suggested fix: for every target, content = ctx.runfiles(transitive_files = files).merge(data_runfiles); the singleton heuristic then becomes unnecessary. The pre-existing comment in common.bzl (~L301-306) that this mirrors is stale and should be fixed too.
| ]), | ||
| ) | ||
|
|
||
| def _test_py_binary_groups(name): |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
major / tests. The only union-equality test uses genrule and filegroup in data, both of which put their outputs in data_runfiles, so it cannot detect the blocker above. Add a Starlark rule with a multi-file DefaultInfo(files = ...) and no runfiles, and one with files != runfiles, in both py_binary data and py_library data, asserting union equality.
|
|
||
| return providers | ||
|
|
||
| def _create_runfiles_groups( |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
major / structure. Groups are produced by re-enumerating every runfiles contribution (required files, pyc variant, extra_default_outputs, stage2, build data, cc/native runfiles, venv pieces) independently of the code that builds the DefaultInfo runfiles (_get_base_runfiles_for_binary, _create_executable). The invariant holds today only by parallel bookkeeping; any future addition to app_runfiles / extra_runfiles silently breaks it with no test coverage.
Suggested direction: build the runtime/venv/app/dep pieces once in _get_base_runfiles_for_binary / _create_executable, return them in runfiles_details / exec_result, and derive both the DefaultInfo runfiles and the groups from those same objects.
| load("//python:py_library.bzl", "py_library") | ||
| load("//python/private:runfiles_groups.bzl", "PyRunfilesGroupsInfo") # buildifier: disable=bzl-visibility | ||
|
|
||
| _tests = [] |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
major / tests. Untested paths: both coarse fallbacks (legacy_create_init = 1, pyc_collection override), py_test, the py_runtime provider, pyi_deps, precompile enabled (implicit pyc vs source selection), py_binary-in-deps, a foreign RunfilesGroupInfo dep, a non-PyInfo dep in py_library (include_plain_dep_py_files), rule targets in srcs, --build_python_zip. The coarse paths were verified to hold via cquery, but nothing in the suite does. Each should get an analysis test asserting _assert_groups_cover_default_runfiles.
|
|
||
| pypi_package, _ = _get_package_and_version(ctx) | ||
| own_entries = struct( | ||
| direct = [library_entry(ctx, pypi_package, own_files.build())], |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
minor / correctness + performance. Every py_library emits an own entry even when it contributes nothing, and the non-PyInfo fallback emits entries with empty runfiles. Verified: every py_binary carries @@rules_python+//python/private:empty rank=-1 n=0 (the debugger alias resolving to a py_library); umbrella libraries and cc_library deps do the same. That's noise for packagers (empty layers unless the consumer filters) and pads the group count that feeds layer_budget merging. Skip own/fallback entries whose content is empty, as collect_src_entries already does.
| }.items(): | ||
| self.assertEqual( | ||
| { | ||
| self._normalize_label(x) | ||
| for x in self._read_file(var) | ||
| if not x.endswith("toolchain_type") | ||
| # The runfiles_groups shim points this attribute at |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
minor / CI. ruff format --check fails on this file ("Would reformat: tests/integration/whl_library/test_contents.py"), so the ruff check is red. Run ruff format on it.
| bazel_10_or_later = str(bazel_major_version > 9), | ||
| )) | ||
|
|
||
| if bazel_major_version >= 9: |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
minor / hermeticity (acceptable as-is, flagging for the record). The shim is keyed on bazel_major_version >= 9 rather than on autoload availability: a Bazel 8 build with --incompatible_autoload_externally= (autoloads off) could load rules_runfiles_group but is stubbed anyway. Fine if documented as "Bazel 9+"; a version-independent key would need Bazel to expose the autoload setting to repository rules.
| merge_affinity = _AFFINITY, | ||
| ) | ||
|
|
||
| def venv_entry(runfiles): |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
nit. The docstring says the venv group goes "late in the ordering", but rank -10 sorts before the per-library groups at rank -1, contradicting the module docstring's stated order (runtime < pypi < venv < libraries < app). Either fix the wording or swap the two ranks.
| transitive.extend(c.transitive) | ||
| return runfiles_groups.entries(direct = direct, transitive = transitive) | ||
|
|
||
| def create_runfiles_group_info(entries, *, executable_group = None): |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
nit / terseness. create_runfiles_group_info is a one-line wrapper around the provider constructor with a single caller; inline it.
| # Mirrors create_py_info: `.py` files of non-PyInfo deps are | ||
| # added to the library's transitive_sources. create_py_info | ||
| # already flattens this same depset in the same analysis, so | ||
| # the to_list() here is a cache hit, not a second flattening. |
There was a problem hiding this comment.
Posted by Fable 5.1 on behalf of @FrankPortman requesting a Claude review.
nit. The comment claims this to_list() is "a cache hit, not a second flattening"; depset flattening isn't memoized per call in a way that makes that reliably true (unverified against Bazel source). Drop the claim and just note the cost mirrors create_py_info for the same rare non-PyInfo dep.
3 participants
Implements #3757 against rules_runfiles_group 0.1.0 (the new entries-depset API).
When enabled, binaries emit these groups:
rules_python#runtime: interpreter + stdlibrules_python#pypi/<name>: one per PyPI package, stable across whl repo namesrules_python#<label>: oneLabelnamed group perpy_libraryrules_python#venv: the binary-specific venv symlinks and support filesrules_python#app: the binary's own code, executable, bootstrap, and build data (executable_group = True)data#<label>: rule targets from data attributes--srcs_as_runfiles) that I feel like is somehow implicated. Libraries propagate entries through a private provider.py_librarymay not get RGI behavior for free without its own shim. We can change this if we are willing to accept a change in semantics aroundunion == default_runfiles(I think).--@rules_runfiles_group//runfiles_group:enabled flag;--//python/config_settings:runfiles_groupsdefaults to following it and can override either way.pyc_collectionaway from the config default, or using legacy implicit__init__.py, falls back to coarse runtime/venv/app grouping (dep entries are computed against the config, so the union would otherwise break).I had Claude patch the Bazel 9 vs Bazel 8 autoloads stuff just to get CI working, since I figure y'all will have a preference for how that is handled, and I can just adapt accordingly.
Fixes #3757
To implement this PR, I first looked at the reference implementation and browsed the
rules_pythonenough to form an opinion on what a sketch might look like. I then steered Claude Fable pretty heavily in plan-mode, given a sketch/pseudocode of how I think it should look. Finally, I used Claude Fable to put finishing touches on comments, documentation, help me think of new test cases, and then stress test some edge cases.I believe I understand this code well enough to respond to feedback and "own" the changes, conceptually, but I want to be transparent about LLM assistance.